Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Iterator::Refresh() #2621

Closed
wants to merge 4 commits into from
Closed

Conversation

siying
Copy link
Contributor

@siying siying commented Jul 20, 2017

Summary: Add and implement Iterator::Refresh(). When this function is called, if the super version doesn't change, update the sequence number of the iterator to the latest one and invalidate the iterator. If the super version changed, recreated the whole iterator. This can help users reuse the iterator more easily.

Test Plan:Add a unit test.

Summary: Add and implement Iterator::Refresh(). When this function is called, if the super version doesn't change, update the sequence number of the iterator to the latest one and invalidate the iterator. If the super version changed, recreated the whole iterator. This can help users reuse the iterator more easily.

Test Plan:Add a unit test.
@facebook-github-bot
Copy link
Contributor

@siying updated the pull request - view changes

@facebook-github-bot
Copy link
Contributor

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@sagar0 sagar0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!!!
lgtm.

HISTORY.md Outdated
@@ -1,4 +1,8 @@
# Rocksdb Change Log
## Unreleased
### New Features
* Add Iterator::Refresh(), which also users to bring the iterator state up-to-date and avoid some initialization costs of the iterator.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be rephrase this as: "... which allows users to update the iterator state ... "

@facebook-github-bot
Copy link
Contributor

@siying updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@siying updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.


InternalIterator* internal_iter = db_impl_->NewInternalIterator(
read_options_, cfd_, sv, &arena_, db_iter_->GetRangeDelAggregator());
SetIterUnderDBIter(internal_iter);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like not the duty of DBIter and feels to me could lead to future bugs if the db_impl called SetIterUnderDBIter in a different way and then we called it in our way.

If we are sure that we will always call SetIterUnderDBIter with an InternalIterator that looks like this

db_impl_->NewInternalIterator(read_options_, cfd_, sv, &arena_, db_iter_->GetRangeDelAggregator());

Then maybe we should have something like

DBIter::SetDBUnderIter(DBImpl* db_impl) {
  InternalIterator* internal_iter = db_impl_->NewInternalIterator(read_options_, cfd_, sv, &arena_, db_iter_->GetRangeDelAggregator());
  SetIterUnderDBIter(internal_iter);
}

And always use this function in db_impl.cc and make SetIterUnderDBIter private

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. Let me try that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying this approach. SetDBUnderIter() needs to pass read_options, cfd, sv, etc. I'm feeling I'm getting things even more complicated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants